Pull Request Review: Features/onboarding

Summary

This PR introduces a comprehensive tutorial/onboarding system using a swipeable modal-based approach. The implementation successfully removes legacy components and provides a cleaner architecture. Overall, this is a well-structured implementation with good documentation, though there are several areas that need attention.

🔴 Critical Issues

1. Missing Test Coverage

Severity: High

There are no tests for the new tutorial system. Given the complexity and critical nature of the onboarding experience, this is a significant gap.

Recommendation:

• Add unit tests for TutorialContext and tutorial logic
• Add integration tests for the tutorial flow (navigation, state management)
• Test edge cases: AsyncStorage failures, rapid state changes, language switching

Files to test:

• src/contexts/TutorialContext.tsx:28-42 - Auto-start logic
• src/components/Tutorial/index.tsx:128-146 - AsyncStorage operations
• src/constants/tutorialContent.ts:76-78 - Language change listener

2. Security: AsyncStorage Error Handling

Severity: Medium
Location: src/contexts/TutorialContext.tsx:28-42, src/components/Tutorial/index.tsx:128-146

AsyncStorage operations can fail silently. While errors are logged, the user experience degrades without feedback.

// Current implementation:
try {
  await AsyncStorage.setItem(TUTORIAL_STORAGE_KEY, 'true');
} catch (error) {
  console.error('[Tutorial] Error saving completion:', error);
}
// ❌ Tutorial closes anyway, even if save failed

Recommendation:

• Show user-facing error alerts for critical AsyncStorage failures
• Consider retry logic for storage operations
• Don't proceed with state changes if storage operations fail

⚠️ Code Quality Issues

3. Memory Leak: Event Listener Not Removed

Severity: Medium
Location: src/constants/tutorialContent.ts:76-78

i18n.on('languageChanged', () => {
  TUTORIAL_CONTENT = getTutorialContent();
});

This event listener is registered at module load time but never removed, causing a memory leak. The listener will persist even after components unmount.

Recommendation:

// Remove this module-level listener entirely
// Instead, use getTutorialContent() directly in components
// OR expose a cleanup function if needed
export const cleanup = () => i18n.off('languageChanged', updateHandler);

4. Mutable Export Anti-pattern

Severity: Medium
Location: src/constants/tutorialContent.ts:73

export let TUTORIAL_CONTENT = getTutorialContent();

Exporting a mutable let variable is an anti-pattern. Consumers may cache stale references.

Recommendation:

// Remove this export entirely - it's unused and problematic
// Keep only getTutorialContent() which is already used everywhere

5. Unsafe Context Usage Pattern

Severity: Low
Location: src/contexts/TutorialContext.tsx:75-92

The fallback behavior in useTutorialContext masks real bugs:

if (!context) {
  console.warn("[TUTORIAL] useTutorialContext called outside TutorialProvider");
  return {
    isTutorialVisible: false,
    startTutorial: () => console.warn("..."),
    // ... silent failures
  };
}

Recommendation:

if (!context) {
  throw new Error("useTutorialContext must be used within TutorialProvider");
}
return context;

This follows React best practices and catches integration errors during development.

6. Hard-coded Magic Numbers

Severity: Low
Location: src/components/Tutorial/index.tsx:414-419

skipHeaderButton: {
  position: 'absolute',
  top: Platform.OS === 'ios' ? 56 : 40, // ❌ Magic numbers
  right: 20,
  // ...
}

Recommendation: Extract to constants:

const SAFE_AREA_TOP_IOS = 56;
const SAFE_AREA_TOP_ANDROID = 40;
const HEADER_PADDING = 20;

🟡 Performance Considerations

7. Unnecessary Re-computation

Location: src/components/Tutorial/index.tsx:92

const pages = React.useMemo(() => buildPages(), []);

Good use of useMemo, but buildPages() calls getTutorialSections() which calls getTutorialSteps() which calls getTutorialContent() - all on every render if i18n changes.

Recommendation:

• Add i18n language as a dependency to useMemo:

const { i18n } = useTranslation();
const pages = React.useMemo(() => buildPages(), [i18n.language]);

8. Large Component File

Severity: Low
Location: src/components/Tutorial/index.tsx (693 lines)

The TutorialOnboarding component is doing too much:

• Rendering 4 different page types
• Pagination logic
• AsyncStorage operations
• Style definitions (230+ lines of styles)

Recommendation: Split into smaller components:

src/components/Tutorial/
├── TutorialOnboarding.tsx (main orchestrator)
├── pages/
│   ├── WelcomePage.tsx
│   ├── SectionHeaderPage.tsx
│   ├── StepPage.tsx
│   └── CompletionPage.tsx
├── TutorialProgressDots.tsx
├── TutorialNavigation.tsx
└── styles.ts (separate file)

📋 Best Practices

9. Documentation Mismatch

Severity: Low
Location: TUTORIAL_SYSTEM.md

The documentation references components that don't exist or are misnamed:

src/components/Tutorial/
├── InteractiveTutorial.tsx      # ❌ Doesn't exist in this PR
├── TutorialTooltip.tsx          # ❌ Doesn't exist in this PR
├── TutorialWelcomeScreen.tsx    # ❌ Doesn't exist (only in old system)
├── index.tsx                     # ✅ Actually TutorialOnboarding

Recommendation: Update TUTORIAL_SYSTEM.md to match the actual implementation:

• Remove references to non-existent tooltip system
• Document the actual swipeable modal approach
• Update component names to match reality

10. Inconsistent Naming

Location: src/components/Tutorial/exports.ts

export { TutorialOnboarding } from './index';

The component is defined in index.tsx but exported via exports.ts. This is confusing.

Recommendation:

• Rename index.tsx → TutorialOnboarding.tsx
• Remove exports.ts and export directly from the component file

✅ Positive Aspects

1. Clean Architecture: Removal of 1700+ lines of legacy code is excellent
2. Good State Management: Context API usage is appropriate for this use case
3. Internationalization: Proper i18n integration throughout
4. User Experience: Swipeable modal with progress indicators is intuitive
5. Accessibility: Good use of platform-specific styling and safe areas
6. Settings Integration: Restart tutorial feature is a nice touch (Settings.tsx:50-65)

🔍 Minor Issues

CategoryView.tsx Changes

Location: src/components/Category/CategoryView.tsx:152-170

The refactoring improves readability but introduces an unused variable categoryElement:

const categoryElement = (
  <Pressable key={...}>
    {/* ... */}
  </Pressable>
);
return categoryElement; // Could just return the JSX directly

This doesn't add value. Consider inlining.

📊 Summary

Recommendations Priority

Must Fix Before Merge:

1. Add test coverage for critical paths
2. Fix AsyncStorage error handling
3. Remove i18n event listener memory leak
4. Update TUTORIAL_SYSTEM.md to match implementation

Should Fix Soon:
5. Change context fallback to throw error
6. Remove mutable TUTORIAL_CONTENT export
7. Add i18n.language to useMemo dependencies

Nice to Have:
8. Extract magic numbers to constants
9. Split large component file
10. Simplify CategoryView changes

Overall Assessment

This PR represents a significant improvement to the onboarding experience with a modern, maintainable architecture. However, the lack of tests and potential memory leak are blocking issues that should be resolved before merging.

Recommendation: Request changes, focusing on test coverage and the i18n listener cleanup.

Great work on the implementation! The new system is much cleaner than the old approach.